Implement gRFC A97: xDS JWT Call Credentials - #12951
Conversation
shivaspeaks
left a comment
There was a problem hiding this comment.
Sending what I have with the high level review. I need to review it in-depth.
| @VisibleForTesting | ||
| static boolean enableXdsFallback = GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_FALLBACK, true); | ||
|
|
||
| @VisibleForTesting |
There was a problem hiding this comment.
Right now this serves no purpose because you're using reflection to set it.
| throw new XdsInitializationException( | ||
| "Invalid bootstrap: server " + serverUri + " with 'jwt_token_file' config missing"); | ||
| } | ||
| String tokenFile = JsonUtil.getString(config, "token_file"); |
There was a problem hiding this comment.
Incorrectly attempts to read "token_file" from config. Should be "jwt_token_file". I'll also recommend to change the variable name to jwtTokenFile.
| } catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
It is okay to use reflection but there is a simpler way. Reflection is inherently slower than direct field access because the JVM has to do extra work at runtime.
I can assume why you made this choice because enableXdsBootstrapCallCreds in BootstrapperImpl.java is not public, hence you chose to go with reflection. We can safely make it public there because it is anyway internal.
| + " {\n" | ||
| + " \"type\": \"jwt_token_file\",\n" | ||
| + " \"config\": {\n" | ||
| + " \"token_file\": \"/var/run/secrets/token\"\n" |
There was a problem hiding this comment.
jwt_token_file* here and below in this file.
| ImmutableMap.of( | ||
| "type", "jwt_token_file", | ||
| "config", ImmutableMap.of( | ||
| "token_file", jwtTokenFile.toAbsolutePath().toString()) |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces JwtTokenFileCallCredentials to load, parse, cache, and refresh JWT tokens from a file, and integrates it into the xDS bootstrap configuration to support call_creds. The review feedback identifies two bugs in JwtTokenFileCallCredentials: a critical issue where valid cached tokens are ignored during backoff after a failed background refresh, and a potential integer overflow when calculating expirationTimeMillis from large exp claims.
| synchronized (lock) { | ||
| if (readState == ReadState.BACKOFF && now >= nextAttemptTimeMillis) { | ||
| readState = ReadState.IDLE; | ||
| } | ||
|
|
||
| if (readState == ReadState.BACKOFF) { | ||
| failStatus = lastReadFailureStatus != null ? lastReadFailureStatus : Status.UNAVAILABLE; | ||
| } else { | ||
| boolean hasValidCache = cachedToken != null && now < expirationTimeMillis; | ||
| boolean expiringSoon = hasValidCache && (expirationTimeMillis - now <= 60000); | ||
|
|
||
| if (hasValidCache) { | ||
| tokenToApply = new TokenInfo(cachedToken, expirationTimeMillis); | ||
| if (expiringSoon && readState == ReadState.IDLE) { | ||
| readState = ReadState.READING; | ||
| triggerRead = true; | ||
| } | ||
| } else { | ||
| if (readState == ReadState.IDLE) { | ||
| readState = ReadState.READING; | ||
| triggerRead = true; | ||
| } | ||
| queuedAppliers.add(applier); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical Bug: Valid cached token is ignored and requests fail during backoff
When a background refresh fails (e.g., due to a transient file read error), the credential enters the BACKOFF state. Under the current implementation, any request made during the backoff window will fail immediately because readState == ReadState.BACKOFF is checked first and causes an immediate failure, completely ignoring the fact that a perfectly valid cached token still exists in cachedToken.
This can cause severe availability issues where healthy, unexpired tokens are not used simply because a background refresh attempt failed.
We should check if a valid cache exists first. If it does, we should always use it and only trigger/skip background refreshes based on the backoff state. If no valid cache exists, only then should we fail-fast during the backoff window.
synchronized (lock) {
boolean hasValidCache = cachedToken != null && now < expirationTimeMillis;
boolean expiringSoon = hasValidCache && (expirationTimeMillis - now <= 60000);
if (hasValidCache) {
tokenToApply = new TokenInfo(cachedToken, expirationTimeMillis);
if (readState == ReadState.BACKOFF && now >= nextAttemptTimeMillis) {
readState = ReadState.IDLE;
}
if (expiringSoon && readState == ReadState.IDLE) {
readState = ReadState.READING;
triggerRead = true;
}
} else {
if (readState == ReadState.BACKOFF && now >= nextAttemptTimeMillis) {
readState = ReadState.IDLE;
}
if (readState == ReadState.BACKOFF) {
failStatus = lastReadFailureStatus != null ? lastReadFailureStatus : Status.UNAVAILABLE;
} else {
if (readState == ReadState.IDLE) {
readState = ReadState.READING;
triggerRead = true;
}
queuedAppliers.add(applier);
}
}
}Add a defensive check to prevent overflow by capping the expiration time at Long.MAX_VALUE if expSeconds is too large. Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces JwtTokenFileCallCredentials to load, parse, and cache JWT tokens from a file, and integrates it into the xDS bootstrap configuration under an experimental flag. It also updates GrpcXdsTransportFactory to support composite call credentials. The review feedback highlights two important safety improvements in JwtTokenFileCallCredentials: using a bounded stream when reading the token file to prevent potential OutOfMemoryError on special devices, and adding a null check on the parsed JSON element to avoid a NullPointerException when handling malformed payloads.
Implements gRFC A97 to support file-based JSON Web Token (JWT) Call Credentials for xDS-enabled clients. Implemented using an experimental coding agent.
Summary of Changes